-
Notifications
You must be signed in to change notification settings - Fork 495
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
10982 10909 Allow using OAI-PMH identifiers as persistent ids of harvested datasets #11049
Conversation
…arvested datasets. #10909. (that whole block of extra checks on the harvest "style" may be redundant by now - I'll think about it)
…ed in this branch.
…r the pid of the imported datasets - merging 2 different approaches implemented in the PRs
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, looks great. Here's some initial feedback.
src/main/java/edu/harvard/iq/dataverse/harvest/client/HarvestingClient.java
Outdated
Show resolved
Hide resolved
src/test/java/edu/harvard/iq/dataverse/api/imports/ImportGenericServiceBeanTest.java
Show resolved
Hide resolved
Co-authored-by: Philip Durbin <[email protected]>
Co-authored-by: Philip Durbin <[email protected]>
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
…added when resolving a conflict with a cherry-picked commit, which of course changes the checksum)
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't run the code but it looks good (docs too) and the "how to test" instructions seem quite clear. Approved.
This comment has been minimized.
This comment has been minimized.
📦 Pushed preview images as
🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
tested and passed in internal - 1670 Files were successfully harvested |
What this PR does / why we need it:
This PR reconciles the 2 implementations of the same feature, mine and @stevenferey 's, hence 2 issue numbers in the PR name. It will only close one of them, #10982; the other issue needs more work on other features being added as part of it and it's going back into "on hold" for now.
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this:
Below is a real life example of an OAI-PMH archive subject to the feature added in this PR.
Create a harvesting client as follows:
harvest url: https://data.progedo.fr/oai
set: None
metadata format: oai_dc
archive type: Generic OAI archive
Most (or all?) of their records will be failing to import when testing with the develop branch prior to this PR. All (or most) should successfully import when testing this branch. As of writing this, having tested just now:
since this is a real, active archive, their holdings are subject to ongoing change; so, your results may vary.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation:
Preview docs at https://dataverse-guide--11049.org.readthedocs.build/en/11049/api/native-api.html#create-a-harvesting-client